New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement optional ring buffer for container logs #28762
Conversation
3af168d
to
7f18df6
Compare
Seems like one reasonable solution. I wonder if the ring buffer should be a fixed size in bytes though (which can be specified). Then this would carry over simply to a compatible interfaces via a pipe, where you could set the pipe buffer size and discard if no space. There are some issues about message boundaries though, you could use datagrams and allow truncation. |
@cpuguy83 maybe embed it to log-opt? |
daemon/logger/ring.go
Outdated
default: | ||
select { | ||
case <-r.ctx.Done(): | ||
case <-r.queue: // Make room on the queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to add a lock to protect against another log message coming in right after this but before we get a chance to queue below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd better try to implement messages ring buffer as a separate data structure with tests and stuff. Probably that was your plan after design review.
I agree with @justincormack about specifying size. However, computing size might be challenging. The number of messages might be okay, too for average use. |
Design SGTM 👍 |
20366c4
to
52626ee
Compare
Perhaps silly comment, but https://github.com/docker/docker/pull/22982/files#diff-0e0dba78caf4a57795625b5ce9943c9eR13 introduced a buffer size, i.e., messages longer than that size are split (even if there's no newline); or is that not relevant here? |
That's true, there is a max. But I think people want to control the buffer based on size not number of messages. |
@justincormack Implementing this in |
52626ee
to
7980fcd
Compare
Although certainly implementing in BytesPipe is significantly faster since we don't need to do any extra allocations, where as each call to I went ahead and added an implementation in |
Best to drop full messages I think. They might be JSON or something and
truncating would be unhelpful.
…On 26 Nov 2016 2:35 p.m., "Brian Goff" ***@***.***> wrote:
@justincormack <https://github.com/justincormack> Implementing this in
github.com/pkg/iotutils.BytesPipe is fairly trivial I think, but
wondering what's better, drop random byte slices or full messages.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28762 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAdcPPpe7Uf5tLMOCojYaCpkvPg0em68ks5rCEOugaJpZM4K6tX6>
.
|
daemon/logger/ring.go
Outdated
// the passed in logger. | ||
func NewRingLogger(driver Logger, logCtx Context, maxSize int64) Logger { | ||
if maxSize < 0 { | ||
maxSize = defaultRingMaxSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: personally, I prefer zero-value for indicating the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I wanted to support a 0 size, which would essentially mean store no more than 1 message.
runconfig/opts/parse.go
Outdated
flags.Var(&copts.loggingOpts, "log-opt", "Log driver options") | ||
flags.StringVar(&copts.volumeDriver, "volume-driver", "", "Optional volume driver for the container") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unintended change, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended, just re-ordered so log-opt is underneath log-driver.
8d54153
to
7980fcd
Compare
Removed the commit with the bytespipe changes. |
@@ -176,7 +176,9 @@ func (l *logStream) Log(msg *logger.Message) error { | |||
defer l.lock.RUnlock() | |||
if !l.closed { | |||
// buffer up the data, making sure to copy the Line data | |||
l.messages <- logger.CopyMessage(msg) | |||
var copy logger.Message | |||
copy = *msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't clone underlying message([]byte), which was whole point IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to clone the message bytes because that won't ever change.
It's the passed in *Message
which gets changed by the log copier since it keeps a single message pointer for all messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copier uses parts of its own buffer for Line:
https://github.com/docker/docker/blob/master/daemon/logger/copier.go#L81
And drivers like fluentd can collect messages for later send, so their Line will be overriden by copier.
Also, I believe this was in comment to CopyMessage :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you're right. I'll bring this back.
daemon/logger/ring.go
Outdated
package logger | ||
|
||
import ( | ||
"context" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be net/context
Cond based buffer is kinda cool. Will review later today. |
7980fcd
to
d15b37d
Compare
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This functionality was added in moby/moby#28762. It allows a user to specify what to do in cases when a log message can't be sent, block the container's stdio or drop messages. Also removes an erronious entry about a `nats` log driver, which does not exist. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Based on reading the source, it looks like there's a serious bug here in the journald and syslog drivers. (Haven't managed to verify yet with an actual built version.) In both of those drivers, you call |
@glasser For instance: line := string(msg.Source) This copies the value of |
I agree. But look at this code this PR put into the syslog driver, which remains there to this day:
That sure seems to read from Source after calling PutMessage. Similar code exists in the journald driver. |
@glasser Yeah, that looks like an issue! |
logger.PutMessage, added in moby#28762 (v17.04.0-ce), clears msg.Source. So journald and syslog were treating stderr messages as if they were stdout. Signed-off-by: David Glasser <glasser@davidglasser.net>
…g options. Documentation based on: * implementation and comments of the [Ring Logger](https://github.com/moby/moby/blob/master/daemon/logger/ring.go#L49) * conversation in implementing [PR 28762](moby/moby#28762)
…g options. Documentation based on: * implementation and comments of the [Ring Logger](https://github.com/moby/moby/blob/master/daemon/logger/ring.go#L49) * conversation in implementing [PR 28762](moby/moby#28762)
This allows the user to set a logging mode to "blocking" (default), or
"non-blocking", which uses the ring buffer as a proxy to the real log
driver.
This allows a container to never be blocked on stdio at the cost of
dropping log messages.
Introduces a new flag to
docker run
anddockerd
,--log-mode
, whichtakes a value of "blocking", or "non-blocking". I chose not to implement
this as a bool since it is difficult to determine if the mode was set to
false vs just not set... especially difficult when merging the default
daemon config with the container config.
One thing to think about here is
logger.Message
could be any size, it just depends on how long the log line was... so it could be almost nothing, or it could be several megabytes which could cause problems in just blindly buffering N number of messages in the ring buffer.We could keep track of this, but this could be tricky and introduces locking into the implementation.